feat: virtualized and optimized diff rendering#2113
Conversation
Generated-By: PostHog Code Task-Id: 821672f9-ed15-4c39-b741-40bce574277a
Prompt To Fix All With AIFix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
packages/git/src/status-stream.ts:35-42
Performance config is duplicated: the same three `-c` flags appear verbatim here and in `PERFORMANCE_CONFIG` in `client.ts`. If one is updated without the other they will silently diverge. Extracting the constants or importing them from a shared location would keep things OnceAndOnlyOnce.
```suggestion
const perfFlags = [
"-c", "core.untrackedCache=true",
"-c", "core.fsmonitor=true",
"-c", "core.preloadIndex=true",
];
const args = [
...perfFlags,
"status",
```
### Issue 2 of 3
packages/git/src/status-stream.ts:111-116
**Collapsed-dir files leak into `untracked`**
When `triggered` fires, the current file is dropped but the first `perDirCap` files that were added before the trigger remain in `untracked`. That means a directory can appear in `overflowedDirs` while still contributing up to `DEFAULT_PER_DIR_CAP` (1 000) entries to `untracked`. Callers such as `getChangedFilesDetailed` iterate `status.untracked` without filtering against `overflowedDirs`, so those 1 000 files still get `countFileLines` called on them — partially re-introducing the performance problem the overflow protection is meant to solve. Consider either retroactively removing already-added entries when a collapse fires, or documenting the "partial-listing" contract explicitly so callers can filter against `overflowedDirs`.
### Issue 3 of 3
packages/git/src/status-stream.test.ts:1-5
**`totalUntrackedCap` truncation path is not covered**
All five tests exercise `perDirUntrackedCap` but none tests `totalUntrackedCap`. The early-return branch (`untracked.length >= totalCap`) and the `totalUntrackedTruncated` flag are therefore uncovered. A test that adds more untracked files than the total cap and asserts `totalUntrackedTruncated === true` and `untracked.length === totalCap` would close this gap.
Reviews (1): Last reviewed commit: "feat: virtualized and optimized diff ren..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const args = [ | ||
| "-c", | ||
| "core.untrackedCache=true", | ||
| "-c", | ||
| "core.fsmonitor=true", | ||
| "-c", | ||
| "core.preloadIndex=true", | ||
| "status", |
There was a problem hiding this comment.
-c flags appear verbatim here and in PERFORMANCE_CONFIG in client.ts. If one is updated without the other they will silently diverge. Extracting the constants or importing them from a shared location would keep things OnceAndOnlyOnce.
| const args = [ | |
| "-c", | |
| "core.untrackedCache=true", | |
| "-c", | |
| "core.fsmonitor=true", | |
| "-c", | |
| "core.preloadIndex=true", | |
| "status", | |
| const perfFlags = [ | |
| "-c", "core.untrackedCache=true", | |
| "-c", "core.fsmonitor=true", | |
| "-c", "core.preloadIndex=true", | |
| ]; | |
| const args = [ | |
| ...perfFlags, | |
| "status", |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/git/src/status-stream.ts
Line: 35-42
Comment:
Performance config is duplicated: the same three `-c` flags appear verbatim here and in `PERFORMANCE_CONFIG` in `client.ts`. If one is updated without the other they will silently diverge. Extracting the constants or importing them from a shared location would keep things OnceAndOnlyOnce.
```suggestion
const perfFlags = [
"-c", "core.untrackedCache=true",
"-c", "core.fsmonitor=true",
"-c", "core.preloadIndex=true",
];
const args = [
...perfFlags,
"status",
```
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
nit but maybe legit i suppose
| if (triggered) { | ||
| collapsedDirs.add(triggered); | ||
| return; | ||
| } | ||
| untracked.push(filePath); | ||
| return; |
There was a problem hiding this comment.
untracked
When triggered fires, the current file is dropped but the first perDirCap files that were added before the trigger remain in untracked. That means a directory can appear in overflowedDirs while still contributing up to DEFAULT_PER_DIR_CAP (1 000) entries to untracked. Callers such as getChangedFilesDetailed iterate status.untracked without filtering against overflowedDirs, so those 1 000 files still get countFileLines called on them — partially re-introducing the performance problem the overflow protection is meant to solve. Consider either retroactively removing already-added entries when a collapse fires, or documenting the "partial-listing" contract explicitly so callers can filter against overflowedDirs.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/git/src/status-stream.ts
Line: 111-116
Comment:
**Collapsed-dir files leak into `untracked`**
When `triggered` fires, the current file is dropped but the first `perDirCap` files that were added before the trigger remain in `untracked`. That means a directory can appear in `overflowedDirs` while still contributing up to `DEFAULT_PER_DIR_CAP` (1 000) entries to `untracked`. Callers such as `getChangedFilesDetailed` iterate `status.untracked` without filtering against `overflowedDirs`, so those 1 000 files still get `countFileLines` called on them — partially re-introducing the performance problem the overflow protection is meant to solve. Consider either retroactively removing already-added entries when a collapse fires, or documenting the "partial-listing" contract explicitly so callers can filter against `overflowedDirs`.
How can I resolve this? If you propose a fix, please make it concise.| import { mkdir, mkdtemp, rm, writeFile } from "node:fs/promises"; | ||
| import { tmpdir } from "node:os"; | ||
| import path from "node:path"; | ||
| import { afterEach, describe, expect, it } from "vitest"; | ||
| import { createGitClient } from "./client"; |
There was a problem hiding this comment.
totalUntrackedCap truncation path is not covered
All five tests exercise perDirUntrackedCap but none tests totalUntrackedCap. The early-return branch (untracked.length >= totalCap) and the totalUntrackedTruncated flag are therefore uncovered. A test that adds more untracked files than the total cap and asserts totalUntrackedTruncated === true and untracked.length === totalCap would close this gap.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/git/src/status-stream.test.ts
Line: 1-5
Comment:
**`totalUntrackedCap` truncation path is not covered**
All five tests exercise `perDirUntrackedCap` but none tests `totalUntrackedCap`. The early-return branch (`untracked.length >= totalCap`) and the `totalUntrackedTruncated` flag are therefore uncovered. A test that adds more untracked files than the total cap and asserts `totalUntrackedTruncated === true` and `untracked.length === totalCap` would close this gap.
How can I resolve this? If you propose a fix, please make it concise.
|
i assume the merge conflicts are from the quill upgrade and this is a result of all of that, but pls double check before merging, the styles are broken on my local at least rn: |
| const args = [ | ||
| "-c", | ||
| "core.untrackedCache=true", | ||
| "-c", | ||
| "core.fsmonitor=true", | ||
| "-c", | ||
| "core.preloadIndex=true", | ||
| "status", |
There was a problem hiding this comment.
nit but maybe legit i suppose
2 participants
When a repository had a lot of untracked changes, the renderer would slow down to a crawl, memory + CPU usage would spike, leading to an eventual renderer crash. For example during a large merge, or a node_modules that was not gitignored
now we're virtualizing the rendering of diff viewers with virtua, streaming back the git status, and limiting diffs at 5000 lines. this makes them faster and not crash your renderer!
I smoke tested that this works in both cloud/local
Created with PostHog Code